IPv6 ACL support for DASH#222
Conversation
|
@marian-pritsak @mukeshmv @chrispsommers Please help review this PR. Thanks! |
dash-pipeline/bmv2/dash_acl.p4
Outdated
| hdr.ipv4.dst_addr : LIST_MATCH @name("hdr.ipv4.dst_addr:dip"); \ | ||
| hdr.ipv4.src_addr : LIST_MATCH @name("hdr.ipv4.src_addr:sip"); \ | ||
| hdr.ipv4.protocol : LIST_MATCH @name("hdr.ipv4.src_addr:protocol"); \ | ||
| meta.is_overlay_ip_v6 : exact @name("meta.is_overlay_ip_v6:is_overlay_pkt_ip_v6"); \ |
There was a problem hiding this comment.
I don't think we need the is_overlay_pkt_ip_v6 field here in each ACL rule since unlike routing we have separate ACL groups for v4 and v6. So depending on whether the pkt is v4 or v6, the correct ACL group is picked up by the pipeline. And all entries in that group will have only v4 or only v6 but not both.
There was a problem hiding this comment.
Hi @mukeshmv, thanks for reviewing it. Are you mixing inbound/outbound ACL groups with v4/v6? Where (or how) in the pipeline it picks up the correct ACL group based on the v4/v6 packet? May be I am missing something here? Thanks!
There was a problem hiding this comment.
@mhanif pipeline picks up v4 or v6 ACL group here
https://github.com/Azure/DASH/blob/main/dash-pipeline/bmv2/dash_pipeline.p4#L120
There was a problem hiding this comment.
Hi @mukeshmv, Yes, you are right. I have made the requested changes and resubmitted to this PR. Thanks.
|
what about bmv2 model, do we have test for the pipeline? |
chrispsommers
left a comment
There was a problem hiding this comment.
The P4 code changes LGTM although am not a dash-pipeline code expert.
I think it'd be a good idea to create a test-case(s) to validate the design (and catch future regressions) since this is a significant enhancement. Perhaps the easiest way is to copy test_saithrift_vnet.py to a new file test_saithrift_vnet_v6.py (and perhaps rename the original with a _v4 in the filename). Then you can edit to create an IPv6 version of the test.
If you're having problems running the tests in your dev environment (i.e. #218) we can have a 1:1 working session, I'd like to help clear it. Also, perhaps this workflow will work, it's also faster for incremental test-case development.
|
Hi @chrispsommers, ran the v6 equivalent of test_saithrift_vnet.py using the suggested workflow. Found few issues in the IPv6 packet processing and fixed them. v6 tests are now passing. Please review the recent changes. Thanks! |
|
@mhanif did you miss to add your v6 version of the vnet test to the commit ? |
|
@mukeshmv @chrispsommers thanks for looking into this. As soon as I submitted the changes last night, I knew that this question will come up and you folks didn't disappoint me :-) I should have mentioned why I didn't include the test. I ran the test by "enabling IPv6" which has disabled to fix the "IPv6 noise" and, hence, I didn't submit the test case file. I do intend to submit the test and should do it ASAP so as to protect any regression. I just have to see how both v4 and v6 can coexist. Do you guys want it to be done as part of this PR or I can work on it and do a separate PR as this might require a little bit of more work? Thanks. |
|
Hi @mhanif , I am OK with merging this now and then adding the IPv6 test later, especially since you've already invested the effort to create it. This general issue of filtering out unwanted IPv6 noise from Linux has to be solved eventually, and a number of folks are aware of it and thinking about it. For the drop case, you can add the expected packet to "veryify_no_packets()" and it will test that there are no instances of the dropped packet, ignoring anything else. The inverse case, verifying you in fact got the wanted packet in the midst of noise, is trickier and merits discussion. Also, thanks for fixing the README gaffes. While you're at it, did you ever try running a single PTF test class from inside the container? If so, and you have an example of such, could you add it to the README? Two people asked about it on the same day and it needs to be documented. |
chrispsommers
left a comment
There was a problem hiding this comment.
LGTM, see general comment also.
@mhanif I don't think you need to enable IPv6 on Linux since we are not using any Linux IPv6 capabilities per-se. My understanding is we are internally crafting a complete VXLAN encapsulated ethernet frame using PTF and sending it as a raw packet on the interface to the bmv2 and then expecting an ethernet frame containing the modified VXLAN encapsulated to egress out on the other interface. Can you try your test with IPv6 disabled ? |
|
Hi @mukeshmv, Yes, you are right. Since our underlay is still v4, I didn't need to enable v6. I tried with the same script which disables v6 and my test worked. However, I am not able to run both v4 and v6 test cases in the same "run". I can run them separately and they pass. I have created an identical v6 test with the class name defined as TestSaiThrift_outbound_udpv6_pkt. It doesn't matter whether I keep them (v4 and v6 classes) in the same file or in different files, after running the v4 test case, the v6 test case just hangs. However if I run them separately by going in to the saithrift client container, I can run them separately and they pass. What am I doing wrong? Below is the test class code. |
@mhanif seems to me to be a PTF thrift client side issue at setup time where it gets blocked before invoking SAI thrift API. As soon as we send a SIGINT (Ctrl-C) to the client, it seems to get unblocked and sends the request to the thrift server just before quitting. We can see that the thrift server correctly programs the P4 table. @reshmaintel @vmytnykx can you take a look ? |
|
Thanks @mukeshmv. Are we good to merge this PR and we can checkin the test case as a separate PR? |
|
As agreed in the WG meeting on Sept. 21 2022, we allowed the remainder of last week for reviews. None appeared, we will merge now. |
* IPv6 ACL support for DASH * Fix: No need to match against the ipv6 pkt type * Fixed few typos * Fixed IPv6 packet processing defects
Added IPv6 ACL support for DASH